-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
to extend OptionalHasElement and OptionalGetElement to accept tensor and sequence types #4421
Conversation
… sequence types Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
input_type_proto = onnx.helper.make_optional_type_proto(tensor_type_proto) | ||
node = onnx.helper.make_node( | ||
'OptionalHasElement', | ||
inputs=['optional_input'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another test-case where inputs=[]
or input=['']
would be useful.
How about test-cases for optional_get_element?
onnx/defs/optional/defs.cc
Outdated
OpSchema() | ||
.SetDoc(OptionalHasElement_ver1_doc) | ||
.SetDoc(OptionalHasElement_ver18_doc) | ||
.Input(0, "input", "The optional input.", "O") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input should be made into an optional input.
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
onnx/backend/test/cmd_tools.py
Outdated
@@ -48,6 +48,8 @@ def prepare_dir(path: str) -> None: | |||
output_dir, f'test_data_set_{i}') | |||
prepare_dir(data_set_dir) | |||
for j, input in enumerate(inputs): | |||
if input is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to understand this: this file is about model inputs/outputs, right, and not about node/op inputs/outputs. For model inputs/outputs there is no possibility of optional-input/output of kind 1 (static), right? I don't understand why this is being triggered. (May be some fix in the model-generator is required if it is adding an unnecessary model-input?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am worried things will break if a model has an input X, and there is no corresponding input-file in the test-folder.
# OptionalHasElement takes a tensor or optional as input | ||
for input_type_proto in [tensor_type_proto, optional_type_proto]: | ||
input_name_options = { | ||
'empty': 'optional_input', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the combination of input_type_proto == tensor_type_proto
and above ('empty': 'optional_input'
) is not a valid combination to test? That might also be triggering the change to cmd_tools.py below?
…input Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
onnx/numpy_helper.py
Outdated
if sys.byteorder == "big": | ||
# Convert endian from big to little | ||
convert_endian(tensor) | ||
if arr is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change still required (after eliminating that test-case)? Wondering why this change is needed.
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
Signed-off-by: Liqun Fu <liqfu@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
…and sequence types (onnx#4421)
Description
to extend OptionalHasElement and OptionalGetElement to accept tensor and sequence types
Motivation and Context
Before the 2 optional ops only accept optional types. This limits its use in cases where input can be both optional and other types. This PR generalizes the 2 ops so it become more flexible.